Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the herb analyze tool to the ERB workflow to catch additional issues beyond what the linter finds, and refactors the Toast component template to use Rails tag helpers instead of manual HTML attribute interpolation.
Changes:
- Refactored
toast.html.erbto usetag.divhelper with proper attribute handling instead of manual HTML string interpolation - Added
herb analyzerstep to the ERB workflow to complement the existing linter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/components/flowbite/toast/toast.html.erb | Refactored from manual HTML div with string interpolation to Rails tag.div helper with **options splat for cleaner, safer attribute handling |
| .github/workflows/erb.yml | Added herb analyzer step to catch additional ERB issues beyond the linter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: herb analyzer | ||
| run: bundle exec herb analyze app/components |
There was a problem hiding this comment.
The bundle exec herb analyze command requires Ruby to be set up and gems to be installed, but this workflow is missing the Ruby setup step. The workflow will fail because bundler is not available.
Add a Ruby setup step before this command, similar to the pattern used in .github/workflows/ruby.yml. You need to add:
- A step to set up Ruby using
ruby/setup-ruby@v1 - Configure
bundler-cache: trueto automatically runbundle install
Without these steps, the bundle exec command will fail because the Ruby environment and the herb gem won't be available.
This seems to catch more/different issues than the linter, so it's worth running both?
> Unexpected error: Herb::Engine::SecurityError: > app/components/flowbite/toast/toast.html.erb:4:36 - ERB output in > attribute names is not allowed for security reasons. - Suggestion: Use > static attribute names with dynamic values instead.
This seems to catch more/different issues than the linter, so it's worth running both?